Skip to content

Do not declare a volume for sshKeySecret if dag persistence is enabled #22913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
potiuk merged 5 commits into from
Aug 6, 2022
Merged

Do not declare a volume for sshKeySecret if dag persistence is enabled #22913

potiuk merged 5 commits into from
Aug 6, 2022

Conversation

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Apr 11, 2022

In scheduler and triggerer components, git-sync-ssh-key volume was created even
when persistence is enabled. This PR fixes that and added tests
in other components to avoid regression

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Apr 11, 2022
jedcunningham
jedcunningham requested changes Apr 12, 2022
View reviewed changes
jedcunningham
jedcunningham requested changes May 5, 2022
View reviewed changes
@github-actions
Copy link

github-actions bot commented Jul 28, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 28, 2022
@ephraimbuddy ephraimbuddy removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 28, 2022
jedcunningham
jedcunningham approved these changes Aug 5, 2022
View reviewed changes
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a conflict, then I think this is ready to go.

@jedcunningham jedcunningham added this to the Airflow Helm Chart 1.7.0 milestone Aug 5, 2022
ephraimbuddy and others added 4 commits August 6, 2022 08:16
@ephraimbuddy
Do not declare a volume for sshKeySecret if dag persistence is enabled
09d4340 
In scheduler and triggerer components, git-sync-ssh-key volume was created even
when persistence is enabled. This PR fixes that and added tests
in other components to avoid regression
@ephraimbuddy @jedcunningham
Apply suggestions from code review
3a0f9a5 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@ephraimbuddy @jedcunningham
Update tests/charts/test_git_sync_scheduler.py
d8b269e 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@ephraimbuddy
Do not declare a volume for sshKeySecret if dag persistence is enabled
fd8ad28 
In scheduler and triggerer components, git-sync-ssh-key volume was created even
when persistence is enabled. This PR fixes that and added tests
in other components to avoid regression
@potiuk potiuk merged commit 3fc895b into apache:main Aug 6, 2022
@ephraimbuddy ephraimbuddy deleted the gitsync-vol branch August 6, 2022 19:01
@jedcunningham jedcunningham mentioned this pull request Oct 10, 2022
Closed
25 tasks
@vrychkov-repay
Copy link

vrychkov-repay commented Jan 5, 2023 •
edited
Loading

This change breaks scheduler deployment when dag persistence, git-sync and git-sync-ssh-key are enabled. In this case, git-sync-ssh-key volume mount will still be generated (see _helpers), without the corresponding git-sync-ssh-key volume.

@potiuk potiuk mentioned this pull request Jan 11, 2023
Closed
@j-mercier
Copy link

j-mercier commented Apr 26, 2023 •
edited
Loading

This change breaks scheduler deployment when dag persistence, git-sync and git-sync-ssh-key are enabled. In this case, git-sync-ssh-key volume mount will still be generated (see _helpers), without the corresponding git-sync-ssh-key volume.

+1
Is it possible to revert this PR? @potiuk @ephraimbuddy @jedcunningham

@j-mercier j-mercier mentioned this pull request Apr 27, 2023
Closed
j-mercier added a commit to j-mercier/airflow that referenced this pull request Apr 27, 2023
@j-mercier
Revert "Do not declare a volume for sshKeySecret if dag persistence i…
6c51e82 
…s enabled (apache#22913)"

This reverts commit 3fc895b
j-mercier added a commit to j-mercier/airflow that referenced this pull request Apr 27, 2023
@j-mercier
Revert "Do not declare a volume for sshKeySecret if dag persistence i…
d488435 
…s enabled (apache#22913)"

This reverts commit 3fc895b

(cherry picked from commit 6c51e82)

We want to mount a volume for sshKeySecret in case git-sync is enabled independently of persistence being enabled or not.
@potiuk
Copy link
Member

potiuk commented Apr 28, 2023

This change breaks scheduler deployment when dag persistence, git-sync and git-sync-ssh-key are enabled. In this case, git-sync-ssh-key volume mount will still be generated (see _helpers), without the corresponding git-sync-ssh-key volume.

+1 Is it possible to revert this PR? @potiuk @ephraimbuddy @jedcunningham

Feel free to open PR which reverts the change, describing why and what happens there.

@dolfinus dolfinus mentioned this pull request Sep 13, 2023
Closed
@dolfinus
Copy link
Contributor

dolfinus commented Sep 13, 2023

Feel free to open PR

Done: #34331

@dolfinus dolfinus mentioned this pull request Jan 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

@dstandish dstandish Awaiting requested review from dstandish

Assignees

No one assigned

Labels

area:helm-chart Airflow Helm Chart

Projects

None yet

Milestone

Airflow Helm Chart 1.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@ephraimbuddy @vrychkov-repay @j-mercier @potiuk @dolfinus @jedcunningham